Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

config parameter disabling forcing sso iframe #463

Merged
merged 6 commits into from
May 24, 2022
Merged

config parameter disabling forcing sso iframe #463

merged 6 commits into from
May 24, 2022

Conversation

jonathanmmm
Copy link
Contributor

@jonathanmmm jonathanmmm commented Mar 29, 2022 •
edited
Loading

haven't checked if it works

@jonathanmmm
Copy link
Contributor Author

would fix #462

@jonathanmmm jonathanmmm force-pushed the fix/462/sso-logins-disable-forced-iframe branch 3 times, most recently from 005b24b to 5d62585 Compare May 2, 2022 22:28
@jonathanmmm
Copy link
Contributor Author

@gary-kim it should build, I only changed one file and it complains about missing package:

[1/2] Removing module @nextcloud/browserslist-config...
error This module isn't specified in a package.json file.
info Visit https://yarnpkg.com/en/docs/cli/remove for documentation about this command.
make: *** [Makefile:18: 3rdparty/riot] Error 1

could you have a look at it?

@jonathanmmm jonathanmmm marked this pull request as ready for review May 2, 2022 22:34
@jonathanmmm
Copy link
Contributor Author

Now it passes all checks, have only changed false to true in the comment in line 59.

@gary-kim
Copy link
Owner

gary-kim commented May 4, 2022

Yeah, my bad. I think the build fixed itself because I updated one of the container images used in the build process.

@gary-kim
Copy link
Owner

gary-kim commented May 4, 2022 •
edited
Loading

So, to add a new config, you will need to add it here with the default value:

riotchat/lib/AppInfo/Application.php

Line 38 in 5b10eed

public const AvailableSettings = [

then make it available to the frontend where you want to use it by adding another one of these:

riotchat/lib/Controller/AppController.php

Line 60 in 5b10eed

$this->initialStateService->provideInitialState(Application::APP_ID, 'disable_custom_urls',

then add it to the admin settings by putting it here (should be similar to the disable custom URLs setting):

riotchat/src/components/AdminSettings.vue

Lines 57 to 67 in 5b10eed

<input
id="disable_custom_urls"
v-model="disable_custom_urls"
type="checkbox"
class="checkbox"
@change="updateSetting('disable_custom_urls')"
>
<label
ref="disable_custom_urls"
for="disable_custom_urls"
>{{ t('riotchat', 'Disable custom URLs') }}</label>

and here

riotchat/src/components/AdminSettings.vue

Line 234 in 5b10eed

"sso_immediate_redirect": loadState('riotchat', 'sso_immediate_redirect') === 'true',

@jonathanmmm
Copy link
Contributor Author

jonathanmmm commented May 4, 2022 •
edited
Loading

Have done it

then make it available to the frontend where you want to use it by adding another one of these:

riotchat/lib/Controller/AppController.php

Line 60 in 5b10eed

$this->initialStateService->provideInitialState(Application::APP_ID, 'disable_custom_urls',

I am not sure if I implemented this part correctly.

@jonathanmmm
Copy link
Contributor Author

Maybe need documentary somewhere, if someone is using custom config.

gary-kim
gary-kim requested changes May 10, 2022
View reviewed changes
src/components/AdminSettings.vue Outdated Show resolved Hide resolved
@jonathanmmm jonathanmmm force-pushed the fix/462/sso-logins-disable-forced-iframe branch from 7704807 to 997c579 Compare May 10, 2022 18:22
gary-kim
gary-kim reviewed May 10, 2022
View reviewed changes
src/main.js Outdated Show resolved Hide resolved
@jonathanmmm
Copy link
Contributor Author

Have done it

then make it available to the frontend where you want to use it by adding another one of these:

riotchat/lib/Controller/AppController.php

Line 60 in 5b10eed

$this->initialStateService->provideInitialState(Application::APP_ID, 'disable_custom_urls',

I am not sure if I implemented this part correctly.

@gary-kim did you check if implemented this part correctly?

Also where would be a good place to document this?

@jonathanmmm jonathanmmm requested a review from gary-kim May 20, 2022 14:30
@gary-kim gary-kim added this to the v0.12.0 milestone May 21, 2022
gary-kim
gary-kim reviewed May 21, 2022
View reviewed changes
Copy link
Owner

@gary-kim gary-kim left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, tested it myself. It does work though the text of the setting and the effect of the setting seem reversed at the moment.

I think we can just fix that and make the setting name a bit clearer and that should be done!

As far as documentation goes, I think just being in the settings will be good enough for now. If questions start being asked, we can make some system for documentation. For now I'd prefer to have it be self-documenting if possible.

src/main.js Outdated
window.localStorage.setItem.apply(this, params);
};
// Setting sso_force_iframe (in config) to true forces iframe even if using SSO or CAS login
if (loadState('riotchat', 'sso_force_iframe') !== 'false') {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (loadState('riotchat', 'sso_force_iframe') !== 'false') {
if (loadState('riotchat', 'sso_force_iframe') === 'false') {

Copy link
Contributor Author

@jonathanmmm jonathanmmm May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think more setting it to !== 'true' would be better, if the value is anything else it will then be treated as false, but your suggestion is easier to read.

src/components/AdminSettings.vue Outdated Show resolved Hide resolved
jonathanmmm
jonathanmmm commented May 24, 2022
View reviewed changes
src/main.js Outdated Show resolved Hide resolved
jonathanmmm and others added 6 commits May 24, 2022 13:33
@jonathanmmm
config parameter forcing iframe, even when using SSO or CAS
dc2cae0 
Signed-off-by: jonathanmmm <[email protected]>
@jonathanmmm
fix comment in src/main.js
73b0f65 
Signed-off-by: jonathanmmm <[email protected]>
@jonathanmmm
includes proposed changes
7d4cc9c 
Signed-off-by: jonathanmmm <[email protected]>
@jonathanmmm @gary-kim
commit suggestion
36eeac2 
Update src/components/AdminSettings.vue

Co-authored-by: Gary Kim <[email protected]>
Signed-off-by: jonathanmmm <[email protected]>
@jonathanmmm
make false a string in condition
9c3635e 
Signed-off-by: jonathanmmm <[email protected]>

Signed-off-by: jonathanmmm <[email protected]>
@jonathanmmm @gary-kim
Apply suggestions
8d14841 
Co-authored-by: Gary Kim <[email protected]>
Signed-off-by: jonathanmmm <[email protected]>
@jonathanmmm jonathanmmm force-pushed the fix/462/sso-logins-disable-forced-iframe branch from 204e6d6 to 8d14841 Compare May 24, 2022 11:37
gary-kim
gary-kim approved these changes May 24, 2022
View reviewed changes
Copy link
Owner

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, tested and it seems to work great! Thank you!

@gary-kim gary-kim modified the milestones: v0.12.0, v0.13.0 May 24, 2022
@gary-kim gary-kim merged commit e98717b into gary-kim:master May 24, 2022
@jonathanmmm jonathanmmm deleted the fix/462/sso-logins-disable-forced-iframe branch June 7, 2022 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gary-kim gary-kim gary-kim approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
v0.13.0
Development

Successfully merging this pull request may close these issues.
2 participants
@jonathanmmm @gary-kim